export targets #403

Karsten1987 · 2020-05-04T18:16:05Z

Karsten1987 · 2020-05-04T18:16:47Z

This is currently work in progress.

I am facing the following CMake error which I have to figure out what's happening:

--- stderr: rosbag2_storage

CMake Error: install(EXPORT "export_rosbag2_storage" ...) includes target "rosbag2_storage" which requires target "tinyxml2_vendor" that is not in any export set.
CMake Generate step failed.  Build files cannot be regenerated correctly.
---
Failed   <<< rosbag2_storage	[ Exited with code 1 ]

I honestly have no real clue why rosbag2_storage should depend on tinyxml2_vendor. I can't see an immediate dependency on it.

Karsten1987 · 2020-05-05T18:19:19Z

The above error could be resolved by ros/pluginlib#190. With that PR, I am putting this PR out of draft mode.

I think the exported targets look decent so far. There's no abosolute path in it. The absolute path for the include directories are coming from other packages as far as I can tell.

set_target_properties(rosbag2_transport::rosbag2_transport PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;/Users/karsten/workspace/ros2/ros2_master/install/include;/Users/karsten/workspace/ros2/ros2_master/install/opt/yaml_cpp_vendor/lib/cmake/yaml-cpp/../../../include"
  INTERFACE_LINK_LIBRARIES "rcl::rcl;rclcpp::rclcpp;rcutils::rcutils;rmw::rmw;rosbag2_compression::rosbag2_compression;rosbag2_compression::rosbag2_compression_zstd;rosbag2_cpp::rosbag2_cpp;yaml-cpp"
)

Karsten1987 · 2020-05-05T22:57:49Z

@emersonknapp @zmichaels11 Maybe you can help me out. I honestly don't really get how that zstd_vendor package is working. Especially the line in here:

rosbag2/zstd_vendor/CMakeLists.txt

Line 52 in 69c5893

ament_export_libraries(zstd)

What is zstd in this case?

From what I can see here zstd does use already exported cmake targets and we should actually just leverage these. However, I am somewhat confused that there is no find_package(zstd) within rosbag2_compression.

zmichaels11 · 2020-05-05T23:01:07Z

@emersonknapp @zmichaels11 Maybe you can help me out. I honestly don't really get how that zstd_vendor package is working. Especially the line in here:

rosbag2/zstd_vendor/CMakeLists.txt

Line 52 in 69c5893

ament_export_libraries(zstd)

What is zstd in this case?

From what I can see here zstd does use already exported cmake targets and we should actually just leverage these. However, I am somewhat confused that there is no find_package(zstd) within rosbag2_compression.

@piraka9011 do you have input on this design choice?

emersonknapp · 2020-05-05T23:02:26Z

I'm not too familiar with the compression code either - it seems like it's just declaring the preexisting library, either system installed or built via ExternalProject - as an ament target?

Karsten1987 · 2020-05-05T23:49:58Z

So I worked around this now by providing a Findzstd.cmake similar to what is done in the sqlite3_vendor package. I've figured out that version 1.4.4 of zstd does not provide any modern CMake logic (neither a find_package so to speak).

piraka9011 · 2020-05-06T03:28:09Z

@Karsten1987 zstd does not have modern CMake exports as you described so instead I exported the library as zstd_vendor and linked using the zstd_vendor_INCLUDE_DIRS and zstd_vendor_LIBRARIES CMake variables.

dirk-thomas · 2020-05-13T20:45:18Z

I would suggest to split the non-export target related changes into a separate PR to make this easier to review.

clalancette

Overall looks pretty good. There are a few cleanups in here that could be punted out to a separate PR just to keep this one a little smaller. I have one question inline.

clalancette · 2020-05-14T13:18:13Z

sqlite3_vendor/CMakeLists.txt

-  install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/sqlite3_install/ DESTINATION ${CMAKE_INSTALL_PREFIX})
+  install(
+    DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/sqlite3_install/
+    DESTINATION .)


All the rest of our vendor packages use ${CMAKE_INSTALL_PREFIX} here:

https://github.com/ros2/spdlog_vendor/blob/3cae65ab249d1ef3c9bfa203497007d8c7ef299f/CMakeLists.txt#L78

https://github.com/ros2/yaml_cpp_vendor/blob/efe04baa5b3903fa238204f291f50f05d4fcdd18/CMakeLists.txt#L62

rosbag2/sqlite3_vendor/CMakeLists.txt

Line 36 in bfa6319

install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/sqlite3_install/ DESTINATION ${CMAKE_INSTALL_PREFIX})

rosbag2/shared_queues_vendor/CMakeLists.txt

Line 38 in bfa6319

DESTINATION ${CMAKE_INSTALL_PREFIX}/include/moodycamel

https://github.com/ros2/tinyxml_vendor/blob/b7a50bf7af9f7ae8c517dd06552533d4e5f4a726/CMakeLists.txt#L37

https://github.com/ros2/rviz/blob/7ca470831304a1cd29ec207f2c07e62ddb8552f1/rviz_assimp_vendor/CMakeLists.txt#L51

https://github.com/ros2/rviz/blob/7ca470831304a1cd29ec207f2c07e62ddb8552f1/rviz_ogre_vendor/CMakeLists.txt#L61

https://github.com/ros2/poco_vendor/blob/3f23e18f8d71e979e03395d860bc3053d3162ae6/CMakeLists.txt#L87

https://github.com/ament/uncrustify_vendor/blob/0291bc988c45846400aca50cb973bf04a4de3d56/CMakeLists.txt#L63

https://github.com/ros/resource_retriever/blob/d6a6818edeebcc3af73b77ebf88d3116d489089e/libcurl_vendor/CMakeLists.txt#L64

That doesn't make the rest of them automatically correct, but I'm not sure why this one is different. Do we need to update all of those to use ., or can we just leave this one as it was?

I did this based on this comment: ros2/console_bridge_vendor#9 (review)

It was changed under the impression that older CMake versions had an issue otherwise. I would guess that was won't the problem was coming from somewhere else. So referring that change sounds reasonable.

Karsten1987 · 2020-05-14T16:12:08Z

I am okay with splitting this PR.

I think I could do the following, which could hopefully be merged individually:

changing to modern cmake targets
make rosbag2 relocatable (specifically their vendor packages)
general touchups

Karsten1987 · 2020-05-14T18:25:05Z

shared_queues_vendor/CMakeLists.txt

@@ -5,8 +5,9 @@ find_package(ament_cmake REQUIRED)

 include(ExternalProject)
 # Single producer single consumer queue by moodycamel - header only, don't build, install
-ExternalProject_Add(singleproducerconsumer
-  DOWNLOAD_DIR ${CMAKE_CURRENT_BINARY_DIR}/single
+ExternalProject_Add(ext-singleproducerconsumer


I deliberately left in these changes because the cmake target of the ExternalProject_Add would collide with add_library.

Karsten1987 · 2020-05-14T18:41:20Z

I would ask for another round of review here. I hope I could strip down the packages well enough to make the changes easier to grasp.

dirk-thomas

(I will leave the actual approval to one of the maintainers.)

dirk-thomas · 2020-05-14T18:55:58Z

zstd_vendor/cmake/Modules/Findzstd.cmake

+    list(APPEND zstd_TARGETS zstd::zstd)
+else()
+    message(FATAL_ERROR "Unable to find zstd")
+endif()


I guess this package doesn't use linters yet.

it does not and but can be added in a follow up PR.

dirk-thomas · 2020-05-14T18:56:02Z

rosbag2_cpp/CMakeLists.txt

 ament_export_dependencies(pluginlib
  rosbag2_storage
  rosidl_runtime_c
  rosidl_runtime_cpp
+  rosidl_typesupport_cpp


How is this related to the change?

From my understanding, when importing a cmake target, all dependencies have to present at cmake's generation step as opposed to using ${<pkg>_LIBRARIES} which is applied during linking. It's therefore important to export all necessary dependencies.

dirk-thomas · 2020-05-14T18:57:25Z

zstd_vendor/cmake/Modules/Findzstd.cmake

+    endif()
+    list(APPEND zstd_TARGETS zstd::zstd)
+else()
+    message(FATAL_ERROR "Unable to find zstd")


Nitpick: I usually try to exit early to avoid indentation, like:

if(NOT zstd_FOUND) message(FATAL_ERROR "Unable to find zstd") endif() ...

Yes, exiting early is preferred.

There was actually another problem in here though, in that it would fail (FATAL_ERROR) regardless whether find_package(zstd) was called with REQUIRED or not. So I've changed that. With this however, I don't see a good way of exiting earlier anymore.

Signed-off-by: Karsten Knese <[email protected]>

Signed-off-by: Knese Karsten <[email protected]>

Signed-off-by: Karsten Knese <[email protected]>

* wip export targets Signed-off-by: Karsten Knese <[email protected]> * correct include dirs Signed-off-by: Karsten Knese <[email protected]> * relative include paths for vendor packages Signed-off-by: Karsten Knese <[email protected]> * cleanup rosbag2_test_common Signed-off-by: Karsten Knese <[email protected]> * cleanup shared_queues_vendor package Signed-off-by: Karsten Knese <[email protected]> * cleanup sqlite3 vendor package Signed-off-by: Karsten Knese <[email protected]> * cleanup zstd_vendor package Signed-off-by: Karsten Knese <[email protected]> * cleanup rosbag2_compression Signed-off-by: Karsten Knese <[email protected]> * touchups for isolated build Signed-off-by: Knese Karsten <[email protected]> * export typesupport Signed-off-by: Karsten Knese <[email protected]> * organize included dependencies Signed-off-by: Karsten Knese <[email protected]> * unknown import target for sqlite3 Signed-off-by: Karsten Knese <[email protected]> * try debugging windows Signed-off-by: Karsten Knese <[email protected]> * remove fatal error warning Signed-off-by: Karsten Knese <[email protected]> * limit amount of change Signed-off-by: Karsten Knese <[email protected]> * use CMAKE_INSTALL_PREFIX Signed-off-by: Karsten Knese <[email protected]> * fatal_error only when required Signed-off-by: Karsten Knese <[email protected]> Signed-off-by: P. J. Reed <[email protected]>

Karsten1987 self-assigned this May 4, 2020

Karsten1987 mentioned this pull request May 5, 2020

link against tinyxml2 correctly ros/pluginlib#190

Merged

Karsten1987 marked this pull request as ready for review May 5, 2020 18:19

Karsten1987 force-pushed the export_targets branch from 84f1798 to 18f8d61 Compare May 5, 2020 23:48

Karsten1987 force-pushed the export_targets branch from 18f8d61 to 2a1616e Compare May 5, 2020 23:56

clalancette mentioned this pull request May 11, 2020

Don't export tinyxml2 internal details. ros/pluginlib#193

Closed

Karsten1987 force-pushed the export_targets branch from 0368bf9 to 64387bf Compare May 13, 2020 01:53

Karsten1987 mentioned this pull request May 13, 2020

make rosbag2 related packages relocatable #402

Closed

clalancette reviewed May 14, 2020

View reviewed changes

Karsten1987 force-pushed the export_targets branch from dbe8c2c to 2b6f5d3 Compare May 14, 2020 18:13

Karsten1987 commented May 14, 2020

View reviewed changes

dirk-thomas reviewed May 14, 2020

View reviewed changes

Karsten1987 added 8 commits May 19, 2020 13:36

wip export targets

519bf15

Signed-off-by: Karsten Knese <[email protected]>

correct include dirs

78e1f56

Signed-off-by: Karsten Knese <[email protected]>

relative include paths for vendor packages

cd69f3a

Signed-off-by: Karsten Knese <[email protected]>

cleanup rosbag2_test_common

26bf9a8

Signed-off-by: Karsten Knese <[email protected]>

cleanup shared_queues_vendor package

e293bfc

Signed-off-by: Karsten Knese <[email protected]>

cleanup sqlite3 vendor package

a0f916e

Signed-off-by: Karsten Knese <[email protected]>

cleanup zstd_vendor package

6eb9153

Signed-off-by: Karsten Knese <[email protected]>

cleanup rosbag2_compression

cd7611e

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 added 9 commits May 19, 2020 13:36

touchups for isolated build

33fa71b

Signed-off-by: Knese Karsten <[email protected]>

export typesupport

d5dcf6a

Signed-off-by: Karsten Knese <[email protected]>

organize included dependencies

9eb4796

Signed-off-by: Karsten Knese <[email protected]>

unknown import target for sqlite3

0e5b4d6

Signed-off-by: Karsten Knese <[email protected]>

try debugging windows

c9d6aef

Signed-off-by: Karsten Knese <[email protected]>

remove fatal error warning

0bdb584

Signed-off-by: Karsten Knese <[email protected]>

use CMAKE_INSTALL_PREFIX

5293e70

Signed-off-by: Karsten Knese <[email protected]>

limit amount of change

8674fb2

Signed-off-by: Karsten Knese <[email protected]>

fatal_error only when required

03702f8

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 force-pushed the export_targets branch from 93465ab to 03702f8 Compare May 19, 2020 20:37

clalancette approved these changes May 19, 2020

View reviewed changes

Karsten1987 merged commit ead6f74 into master May 20, 2020

delete-merged-branch bot deleted the export_targets branch May 20, 2020 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

export targets #403

export targets #403

Karsten1987 commented May 4, 2020 •

edited

Loading

Karsten1987 commented May 4, 2020 •

edited

Loading

Karsten1987 commented May 5, 2020

Karsten1987 commented May 5, 2020

zmichaels11 commented May 5, 2020

emersonknapp commented May 5, 2020

Karsten1987 commented May 5, 2020

piraka9011 commented May 6, 2020

dirk-thomas commented May 13, 2020

clalancette left a comment

clalancette May 14, 2020

Karsten1987 May 14, 2020

dirk-thomas May 14, 2020

Karsten1987 commented May 14, 2020

Karsten1987 May 14, 2020

Karsten1987 commented May 14, 2020

dirk-thomas left a comment

dirk-thomas May 14, 2020

Karsten1987 May 14, 2020

dirk-thomas May 14, 2020

Karsten1987 May 14, 2020

dirk-thomas May 14, 2020

Karsten1987 May 14, 2020

export targets #403

export targets #403

Conversation

Karsten1987 commented May 4, 2020 • edited Loading

Karsten1987 commented May 4, 2020 • edited Loading

Karsten1987 commented May 5, 2020

Karsten1987 commented May 5, 2020

zmichaels11 commented May 5, 2020

emersonknapp commented May 5, 2020

Karsten1987 commented May 5, 2020

piraka9011 commented May 6, 2020

dirk-thomas commented May 13, 2020

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented May 14, 2020

Choose a reason for hiding this comment

Karsten1987 commented May 14, 2020

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented May 4, 2020 •

edited

Loading

Karsten1987 commented May 4, 2020 •

edited

Loading